fix(corekit): reparent the unblocked non-registry enums onto EnumLookup (#877) - #921

Cross-review on Opus returned NEEDS CHANGES (author was Sonnet). Both findings were prose, both are now fixed — new head 21c9b2830.

The code was sound; two comments asserted something measurement refutes. I re-derived both myself on 3766c3c09 versus the branch before acting.

What was wrong. The delegation is exception-compatible for every key the signature admits (int | str), but not outside it, and two comments claimed otherwise:

cls(key) accepts whatever int equality accepts, which is how a float resolves. In-contract keys are byte-identical including messages — exact-case hit, wrong-case hit, name miss, 6, 0, -1, 999999, True, b'tcp', and a Criticality instance.

No caller can reach any of it: the only live sites are TransportProtocol.get(proto.lower()) and three Criticality.get(...) on pycrate-decoded str/int. And it is what every other EnumLookup subclass already does, so it is alignment, not a regression. Both comments now say so explicitly instead of over-claiming, and the apptype.py edit is mirrored in pcapkit/vendor/reg/apptype/apptype.py's codegen template so regeneration cannot drop it.

Also corrected: the pre-fix failure count is 31 FAIL/ERROR lines over 26 distinct Class.method identities, not 30. Re-derived on a clean origin/main worktree — failures=19, errors=12.

Verified after the amend: 51 tests across test_enum_lookup_reparent_877_unit, test_vendor_reg_apptype_generator_unit and test_const_enum_get all pass. review: pending stays until the verdict is re-confirmed at this head.

GOOD TO GO at head 21c9b2830 — the Opus reviewer's confirmation pass, re-verified by me.

The two load-bearing claims, measured independently:

• Nothing but prose changed. git diff db5953a5d 21c9b2830 is 3 hunks in 3 files, 45 insertions / 12 deletions, and filtering the added lines for anything that is not a comment or docstring prose returns empty. The reviewer went further with an AST comparison: the const file is AST-identical outright, ngap.py differs only in a docstring string constant, and the vendor file differs only because its text lives inside an f-string literal the AST sees as a JoinedStr.
• The vendor template reproduces the committed comment. This is the one that mattered, since pcapkit/const/reg/apptype/apptype.py is generated. The get region is 93 lines in both files and identical once the template's doubled braces are un-doubled — the added prose introduces no braces, no new placeholder, no backslashes and no stray """. So regeneration cannot silently revert it.

Worth recording from the reviewer, correcting its own earlier framing: the template is a lambda … : f'''…''', compiled at import time rather than a deferred .format(), so a broken brace would surface as an ImportError/SyntaxError — and the module imports clean. TransportProtocol.get( still occurs exactly once in each file, so tests/vendor/test_vendor_reg_apptype_generator_unit.py:205 is unaffected.

Behaviour is unchanged from db5953a5d: the probe gives the same results at both heads, member tables still 5 and 3. 51 tests pass across the three affected files.

One item remains UNVERIFIED and is not blocking: the ftp.py / httpv1.py / httpv2.py 100% → 100% coverage claim. Confirming it needs those protocols' own full suites, which was outside the review budget, and the files are 10-line reparents with no new branches.

Unpublished and unmerged — yours to merge. Note the merge-order constraint recorded on #922.

We may want to revise if their overrides after subclassing EnumLookup are still necessary.

Ruling recorded, verbatim:

We may want to revise if their overrides after subclassing EnumLookup are still necessary.

Measured before answering, on 21c9b2830, by comparing each override against what the class would inherit with no override at all (EnumLookup.get.__func__(cls, key)):

So neither override is redundant — but each is down to a small, nameable job. TransportProtocol.get earns its keep twice: case-insensitivity ('TCP' resolving at all) and the KeyError → ValueError conversion on a name miss. Criticality.get earns it once, for the conversion only — it is deliberately case-sensitive, which the base already is, so that half is now redundant in effect if not in code.

Deleting either would be a public behaviour change, not a cleanup: a name miss would start raising KeyError instead of ValueError, and TransportProtocol would stop accepting 'TCP'.

Which leaves a real design question, and it is yours: is the ValueError-on-name-miss shape the house contract that :meth:EnumLookup.get should adopt, or is it a per-class quirk these two should keep re-raising for? If the base converted, both overrides would shrink — Criticality's would disappear entirely and TransportProtocol's would be the .lower() and nothing else. That is a change to the base's contract and affects every subclass, so I am not guessing at it.

Labelled needs: decision. #921's verdict stands and its head is unchanged — this is a follow-up reduction, not a defect in it, so it is still mergeable as-is. I am sweeping every concrete EnumLookup subclass for what it raises on a name miss, so the answer comes with a population count rather than two data points.

Either ValueError or KeyError, that's depending on how stdlib's Enum would raise on these circumstances. And we should raise one from pcapkit.utilities.exceptions rather builtin exceptions.

Ruling recorded, verbatim:

Either ValueError or KeyError, that's depending on how stdlib's Enum would raise on these circumstances. And we should raise one from pcapkit.utilities.exceptions rather builtin exceptions.

Measured on Python 3.14.7, so the shape the ruling selects is concrete: E['nosuch'] → KeyError: 'nosuch', E(999) → ValueError: 999 is not a valid E, E(None) → ValueError. A name miss is KeyError-derived, a value miss is ValueError-derived.

Two consequences, and one gap I did not expect:

• EnumLookup.get already has the right shape and the wrong provenance — bare builtin KeyError and ValueError.
• The conversion in these two overrides is therefore the wrong direction. TransportProtocol.get and Criticality.get both turn the base's KeyError name-miss into ValueError; under this ruling it should stay KeyError-shaped. So the answer to your earlier question is that the overrides are not merely reducible — one half of each is actively contrary to the contract you just set.
• pcapkit/utilities/exceptions.py has no KeyError-derived class. The value half exists as EnumValueError(BaseError, ValueError) at line 374, but every one of that module's exceptions derives from TypeError, AttributeError, ValueError, IOError, IndexError, FileExistsError or FileNotFoundError. Something like EnumKeyError(BaseError, KeyError) has to be added before get can comply.

Filed as #923 (fix, breaking, blocked) with the full scope: add the exception, change the base, delete the conversions — Criticality.get disappears entirely, TransportProtocol.get reduces to the .lower() — and update conventions.rst's registry-protocol section. Blocked on this PR and #922 merging, since all three touch the same files. The census I mentioned will post its counts there.

Removing needs: decision: the direction is settled and the work is tracked elsewhere.

Also rebased onto ada85bbfc now that #919 has merged — this PR was BEHIND. New head 3e7d73de0, one commit, no conflicts, and the 51 tests across test_enum_lookup_reparent_877_unit, test_vendor_reg_apptype_generator_unit and test_const_enum_get still pass. The rebase carried no content change, so review: good-to-go stands.

So the overloads are still legit? Or they will be fixe/updated in #923?

Both: legitimate as shipped here, then reduced in #923. Measured rather than assumed.

Legitimate now. Each override still does something the inherited base does not, so nothing in this PR ships dead code:

• TransportProtocol.get — case folding, and the KeyError → ValueError conversion.
• Criticality.get — the conversion only.

#923 removes the conversion, and that is what changes each of them. Under the ruling — stdlib shape, pcapkit.utilities.exceptions provenance — a name miss stays KeyError-derived, so the conversion is the part that goes:

• TransportProtocol.get survives, shrunk to the .lower() and nothing else. Case-insensitivity is not in the base and is live public behaviour: get('TCP') resolves today and raises KeyError without the override.
• Criticality.get can be deleted outright. Its body becomes a pure pass-through once the conversion goes, and I checked the one thing that could have made that unsafe — whether deleting it would change default handling. It does not. The base honours a member-valued default identically to the override's forwarding:

inherited base:  Criticality.get(nosuch)           -> KeyError: nosuch
                 Criticality.get(nosuch, <member>) -> <Criticality.reject: 0>
                 Criticality.get(nosuch, 99)       -> KeyError: nosuch   (non-member default not honoured)
this override:   Criticality.get(nosuch, <member>) -> <Criticality.reject: 0>   (identical)

The only thing lost by deleting it is the narrower int | str | Criticality key annotation, which is a typing nicety rather than behaviour.

One thing I went looking for and did not find, so it is not a defect to fix: I expected the default parameter this PR adds to contradict the original docstring's argument that "a default parameter here would have to be ignored, and one that is declared, documented and ignored is worse than one that is absent". That paragraph was correctly rewritten — it now explains that default is forwarded verbatim and resolves through _value2member_map_ rather than the constructor, so passing one still cannot mint a fourth value on a closed ASN.1 ENUMERATED. Consistent as it stands.

So: merge this as-is, and #923 does the reduction. I have recorded the above in #923's scope so the two-into-one shrink is not rediscovered there.

Correction to this PR's description, for the record. It says pcapkit/protocols/internet/{esp,mh}.py were "still held by #913/#904 respectively". The #904 half is wrong.

Measured from the API: #904 touched neither file. Its file list is pcapkit/protocols/internet/ipv6.py, the ipv6_generic_ext set, and tests/protocols/internet/test_mh_unit.py. The PR that actually held both esp.py and mh.py is #924 (issue #917), created 15:49:56Z and merged 17:13:44Z as 93cf940b3 — overlapping this PR's own window (created 14:39:40Z, merged 16:06:04Z).

The #913 half is correct: it held pcapkit/const/ftp/command.py and its vendor template, merged 13:54:26Z.

This does not change anything this PR did — the 11 classes it re-parented were the right set, and the seven it deferred were genuinely contended. Surfaced while reviewing #932, which finished those seven and had inherited the same misattribution from here.